Image Build/Push CI #875
Merged
Image Build/Push CI #875
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Automatic image build and push
|
Intended to close e-mission/e-mission-docs#752. |
shankari
requested changes
Sep 2, 2022
There was a problem hiding this comment.
@aGuttman Three high level comments:
- I understand that the dockerfile needs to be in the root directory for the scripts to work, but let us please not put the other scripts there.
- instead we should put them into
./setupor./setup/dockeror./docker - Putting files into the root directory of the repo should be avoided unless we have no alternative.
- instead we should put them into
- When do you plan to remove the corresponding files from the
e-mission-dockerrepo? Having duplicate files just confuses people downstream. - Please indicate "testing done"
|
I thought I would wait until this PR is accepted to remove the files from the docker repo. I guess I can make a PR removing then ahead of time though. |
I just asked when you would remove the dependency. OK to remove them later, but I would suggest creating a cleanup issue so we can track pending issues and they are not just dropped. |
Some reorganization to address PR comments. No logic changes.
shankari
added a commit
to shankari/em-public-dashboard
that referenced
this pull request
Sep 7, 2022
…ronments
In both:
- use the `dev-only` image and clone from Andrew's repo
- this is the workaround for
e-mission#53 until
e-mission/e-mission-server#875 is merged
In the dev files:
- force a version for the http server
- build the viz_scripts image instead of using a pinned image
- This subsumes e-mission#53
- Since the context is now `viz_scripts` and not `viz_scripts/docker` to
maintain consistency with the prod server, change the paths to the various files
In the prod files:
- Remove the mounted code directories
- Change the dockerfiles to copy files into the image
Testing done: This builds, but DOES NOT WORK
Checking this in temporarily so that we can highlight the fix when we do get it to work
```
Building notebook-server
[+] Building 865.9s (20/20) FINISHED
=> [internal] load build definition from Dockerfile 0.0s
=> => transferring dockerfile: 973B 0.0s
=> [internal] load .dockerignore 0.1s
=> => transferring context: 2B 0.0s
=> [internal] load metadata for docker.io/emission/e-mission-server.dev.server-only:4.0.0 0.0s
=> CACHED [1/8] FROM docker.io/emission/e-mission-server.dev.server-only:4.0.0 0.0s
=> [internal] load build context 0.1s
=> => transferring context: 112.46kB 0.0s
=> CACHED [ 2/15] ADD docker/environment36.dashboard.additions.yml / 0.0s
=> CACHED [ 3/15] RUN bash -c "/clone_server.sh" 0.0s
=> CACHED [ 4/15] RUN /bin/bash -c "cd e-mission-server && source setup/activate.sh && conda env update --name emission --file setup/environment36.notebook.additions.yml" 0.0s
=> CACHED [ 5/15] RUN /bin/bash -c "cd e-mission-server && source setup/activate.sh && conda env update --name emission --file /environment36.dashboard.additions.yml" 0.0s
=> [ 6/15] RUN mkdir -p /usr/src/app/saved-notebooks 0.4s
=> [ 7/15] WORKDIR /usr/src/app/saved-notebooks 0.0s
=> [ 8/15] COPY auxiliary_files . 0.0s
=> [ 9/15] COPY bin . 0.1s
=> [10/15] COPY conf . 0.1s
=> [11/15] COPY *.ipynb . 0.1s
=> [12/15] COPY *.py . 0.1s
=> [13/15] ADD docker/start_notebook.sh /usr/src/app/start_notebook.sh 0.0s
=> [14/15] RUN chmod u+x /usr/src/app/start_notebook.sh 0.5s
=> [15/15] ADD docker/crontab /usr/src/app/crontab 0.1s
=> exporting to image 35.9s
=> => exporting layers 35.9s
=> => writing image sha256:61830a6f2563e657350d435c6c512f32f31c0c6e1cc13a9ad5a9bc3b57d66360 0.0s
=> => naming to docker.io/em-pub-dash-prod/viz-scripts 0.0s
Building dashboard
[+] Building 2.9s (10/10) FINISHED
```
```
notebook-server_1 | /usr/src/app/e-mission-server /usr/src/app/saved-notebooks
notebook-server_1 | DB host = db
notebook-server_1 | cp: cannot stat 'e-mission-server/conf/storage/db.conf': No such file or directory
notebook-server_1 | Web host = 0.0.0.0
notebook-server_1 | /usr/src/app/saved-notebooks
notebook-server_1 | cat: saved-notebooks/conf/storage/db.conf: No such file or directory
notebook-server_1 | /usr/src/app/start_notebook.sh: line 27: pushd: e-mission-server: No such file or directory
notebook-server_1 | /usr/src/app/start_notebook.sh: line 29: setup/setup.sh: No such file or directory
```
|
@aGuttman I still need to see the resolution to the conf issue and the testing on the |
This is done for the repo in settings>secrets>actions. There should a "repository secrets" box where you create and enter the values for DOCKER_USER and DOCKER_PASSWORD. |
|
Also, @aGuttman is publishing the images in dockerhub, not in the GitHub container repo, so this is not true.
|
shankari
approved these changes
Sep 12, 2022
|
@aGuttman I have approved these changes in accordance with our discussion today. Before I merge, I would like:
|
|
From Jianli
|
|
@aGuttman ok so I am going to wait for a couple of days to see if we hear from cloud services, otherwise, I will merge and we can deal with the registry selection as part of the cleanup issue. For reference, the reason that we may want to not use dockerhub is that:
This may not be an issue since hopefully we will change the server at least once every 6 months. But we might also want to see if there in a NREL paid version of docker that we can use to push instead of the e-mission account. |
|
@aGuttman would be good if you could do the testing in the next couple of days so we are ready to merge.
Can you outline your testing plan here? |
|
Proposed testing plan:
In terms of expectations, I would like to see the steps that you did. So
|
|
Having trouble with the cron job test. |
|
in general, while debugging docker images, you want to run a different command and then "ssh" into the container using and then run commands manually to figure out what is going on and how you need to change the start script. |
|
For
Changes to `webapp` and `analysis` files
Using freshly built images, not cached ones
Output of `docker compose up`, looks pretty clean to me |
|
@aGuttman Thanks for getting this done.
I think we should be ready to merge everything after that Please link the new PRs (at least e-mission-docker) to the related issue e-mission/e-mission-docs#752 for the record. |
|
@shankari |
|
@aGuttman |
|
Testing changes with First thing, Error from
|
|
I think you can make the same changes as you did to the |
|
|
shankari
requested changes
Sep 22, 2022
shankari
reviewed
Sep 22, 2022
shankari
approved these changes
Sep 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Creates a GitHub actions workflow to automatically build and push new docker images on commits to
master,gis-based-mode-detectionandjoin_redirect_to_static.Images are named after the repo and tagged with their branch and a timestamp. Requires
DOCKER_USERandDOCKER_PASSWORDenvironment variable secrets to be set the push the image to dockerhub.Adapts the
Dockerfilealong withclone_sever.shandstart_script.shfrome-mission/e-mission-dockerfor use inside the server repo.